redo destroy pg#435
Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## stable/v4.x #435 +/- ##
==============================================
Coverage ? 53.29%
==============================================
Files ? 36
Lines ? 5404
Branches ? 677
==============================================
Hits ? 2880
Misses ? 2226
Partials ? 298 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
JacksonYao287
force-pushed
the
fix-issues-of-destroy-pg
branch
from
0b5e4a4 to
c686ce2
Compare
| @@ -499,7 +503,7 @@ void ReplicationStateMachine::write_snapshot_obj(std::shared_ptr< homestore::sna | |||
| if (home_object_->pg_exists(pg_data->pg_id())) { | |||
| LOGI("pg already exists, clean pg resources before snapshot, pg={} {}", pg_data->pg_id(), log_suffix); | |||
| // Need to pause state machine before destroying the PG, if fail, let raft retry. | |||
There was a problem hiding this comment.
comments out of date, as well as we dont have a branch that returns false as of now.
There was a problem hiding this comment.
let`s remove this out-of-date comments after addressing other comments for this PR
JacksonYao287
force-pushed
the
fix-issues-of-destroy-pg
branch
from
c686ce2 to
41de75e
Compare
| // error. but actually, this is not a problem. since before we starting pg_destroy in baseline resync, | ||
| // m_rd_sb->last_snapshot_lsn will be persisted upto the snapshot.get_last_log_idx(). then all the log less than | ||
| // or equal to m_rd_sb->last_snapshot_lsn will not be replayed or committed after recovery. so, the concern is |
There was a problem hiding this comment.
@xiaoxichen in baseline resync case , before we start destroying pg, we will m_rd_sb->last_snapshot_lsn upto snapshot.get_last_log_idx(). then raft_repl_dev#need_skip_processing will help us skipping replaying all the logs in recovery path(so that we will not hit those destroyed resources , like pg_index_table, etc.). so we don`t need wait for all the appended log to be committed in pg_destroy for BR case
There was a problem hiding this comment.
so basically you reverted your changes
There was a problem hiding this comment.
I dont get the point of this comments, you said for br no need to redo destroy PG but we do it anyway.
The concern of log replay vs destroy is not valid here ... if we reach here the log replay had been done...If we want to record the thinking why waiting for log commit is not needed, better rephrase this paragraph and move it to destroy_pg
Similar for L1043-1052, those lines explains the situations that a PG can be destroy, better to move to destroy_pg rather than here, especially we use same action in recovery path , for all source
xiaoxichen
left a comment
xiaoxichen
left a comment
There was a problem hiding this comment.
LGTM aside from inline comments cleanup.
Try to use LLM to polish the language a bit.
3 participants
No description provided.